Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include chart annotations as event metadata #514

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

eljulians
Copy link

@eljulians eljulians commented Aug 10, 2022

This PR extends the Helm release controller to include Chart annotations in the registered event as metadata, along with the Chart revision already included.

This is my first Go code, so feel free to give opinionated feedback :)

Rationale

Use case

With the current notifications, is hard to be aware of what exactly was deployed, as just the Helm chart revision is included in the payload. If I wanted to know what specific change (or changeset) has been rolled out, it wouldn't be possible with the current setup. A workaround could be to abuse the chart version semver, but of course with several drawbacks, like needing to keep a 1-1 relationship between the char and app versions, having to come up with some specific encoding, having it to decode on the other end if a generic webhook receiver has been configured, and just probably being a bad practice.

It's probably reasonable to be able to plug some arbitrary data into the event delivered by Flux, specially considering that the Helm charts already provide annotations for this. I had a look into the open issues and it's something that was mentioned several times (#412 and #256 just to mention a couple of examples).

By including the chart annotations as part of the metadata, users can enrich their notifications as they wish by including the data they consider necessary for their own use cases.

Doing it with the chart annotations, the user experience doesn't change, as the chart needs to be updated for making a release anyways, and the data can be set at that point; or just left it empty otherwise if it's not needed, as they're optional.

Changes

The calls to the event function from reconcile are done passing nil as the chart is not loaded yet at that point.

The annotations are merged in the root of the event map instead of having a nested structure, for simplicity, e.g. the payload looks like:

{
    "revision": "1.0.2",
    "annotation_key1": "annotation_key2"
}

Instead of

{
    "revision": "1.0.2",
    "annotations": {
        "annotation_key1": "annotation_key2"
    }
}

It's assumed that the annotations is a plain string:string map, with no nested structures, according to the Helm specification. Setting annotations in a different format will make the Helm release fail itself (ArtifactFailed error), with no registered event by the Helm release controller, so there's no point on checking for these.

Summary of changes

  • Add metadata *chart.Metadata parameter to event function in controllers/helmrelease_controller.go
  • In reconcile function, modify the calls to event in to pass a nil value for this new param
  • In reconcileRelease function, modify the calls to event in passing chart.Metadata
  • In event function, initialize empty meta map at the beginning
  • In event function, iterate through the newly added chart.Metadata parameter if not null, and merge the data to the meta map that gets send as part of the event

Testing

Manual testing

Below are the steps I followed for testing it with a local k8s cluster. The infrastructure and source code repositores I've used are private, but they're just the minimal for configuring helm releases and alerts, and a simple API server to make test releases for, respectively. I can provide more details if needed.

The source code contains an endpoint for which the HelmRelease notifications are configured for, which logs the request payload, so that there's no need for another service for the notifications in order to check the payload.

The following versions of the tools were used:

  • minikube v1.25.2
  • kubectl client v1.24.2
  • flux v0.31.1

Setting the local cluster up

Start minikube:

minikube start

(Once the cluster is up, the source code docker image needs to be built loaded to minikube).

On the infra repository, bootstrap Flux:

flux bootstrap gitlab \
  --owner=julenpardo \
  --hostname=gitlab.hostname.com \
  --repository=fleet-fastapi-example \
  --branch=main \
  --path=./clusters/my-cluster \
  --personal

Running the Helm controller

Scale the already running Helm controller down:

kubectl -n flux-system scale deployment/helm-controller --replicas=0

Forward the source and notification controllers:

kubectl -n flux-system port-forward svc/source-controller 8081:80
kubectl -n flux-system port-forward svc/notification-controller 8082:80

Run the controller specifying the event address. Apparently the controller doesn't look for an environment variable for it unlike with the source controller, so I had to figure out how to do it, and I manually tweaked the Makefile the following way for testing purposes (I'm not sure of what the best workflow for this is):

diff --git a/Makefile b/Makefile
index f4bfbf3..18d5306 100644
--- a/Makefile
+++ b/Makefile
@@ -37,7 +37,7 @@ manager: generate fmt vet
 
 # Run against the configured Kubernetes cluster in ~/.kube/config
 run: generate fmt vet manifests
-	go run ./main.go
+	go run ./main.go --events-addr=http://localhost:8082
 
 # Install CRDs into a cluster
 install: manifests

Finally, run the controller:

export SOURCE_CONTROLLER_LOCALHOST=localhost:8081 
make run

Test: watch logs for Helm releases

Watch the logs of the fastapi-example pod:

kubectl logs -n fastapi -f $(kubectl get pods -n fastapi -o=name)

In the fastapi-example project directory, define a new Helm release by bumping version in charts/Chart.yaml. Also add some annotations, e.g.:

annotations:
    env: dev
    commit_sha: eb93c4033dc402de9d97c3d345c6ef1bce7d125f
    repo_host: https://github.com
    repo_owner: julenpardo
    repo_name: fastapi-exapmle

Commit, push and wait for the Helm controller to make the upgrade. Once done, the notification endpoint should have been hit, and the fastapi-example log output should show the payload including the annotations within metadata:

{
    "involvedObject": {
        "kind": "HelmRelease",
        "namespace": "flux-system",
        "name": "fastapi",
        "uid": "e92d963d-3c45-4a53-a2c5-11df902fe1f5",
        "apiVersion": "helm.toolkit.fluxcd.io/v2beta1",
        "resourceVersion": "3551"
    },
    "severity": "info",
    "timestamp": "2022-08-04T17:23:50Z",
    "message": "Helm upgrade succeeded",
    "reason": "info",
    "metadata": {
        "commit_sha": "eb93c4033dc402de9d97c3d345c6ef1bce7d125f",
        "env": "dev",
        "repo_host": "https://github.com",
        "repo_name": "fastapi-exapmle",
        "repo_owner": "julenpardo",
        "revision": "1.0.2"
    },
    "reportingController": "helm-controller",
    "reportingInstance": "jpardo-le-XXXXX"
}

(The Helm upgrade has started has been omitted for convenience, but the same metadata is included in the payload).

Make another release this time without any annotations, to check that everything is still okay without them, and check the logs:

{                                                                                                                                 
    "involvedObject": {                                                                                                           
        "kind": "HelmRelease",                                                                                                    
        "namespace": "flux-system",                                                                                               
        "name": "fastapi",                                                                                                        
        "uid": "e92d963d-3c45-4a53-a2c5-11df902fe1f5", 
        "apiVersion": "helm.toolkit.fluxcd.io/v2beta1",
        "resourceVersion": "5850"
    },                 
    "severity": "info",                                          
    "timestamp": "2022-08-04T17:40:01Z",
    "message": "Helm upgrade succeeded",
    "reason": "info",
    "metadata": {          
        "revision": "1.0.3"
    },                                                           
    "reportingController": "helm-controller",
    "reportingInstance": "jpardo-le-XXXXX"
}  

Unit and regression testing

I didn't add any automated tests mainly because currently there's no coverage for the flow for reconcileRelease, so I'm not sure of what would the best approach be to tackle this, and not end up being a pull request of adding tests rather than a feature. But suggestions are welcome.

@eljulians eljulians marked this pull request as draft August 10, 2022 12:19
@eljulians eljulians force-pushed the julenpardo/chart-annotations-notification-metadata branch from 3e11ad1 to d82ad5e Compare August 10, 2022 12:21
Extend the registered event after the Helm reconciliation to include
the chart annotations (if any) in the existing metadata field of the
event body. `event` function defines now a new
 `metadata *chart.Metadata` parameter with this metadata.

The fields defined in the chart annotations are merged to the already
defined `meta` map in the `event` function, along with the already
existing `revision` field. These fields are merged at the root level;
so the `meta` map will have n + 1 fields, where n is the number of
annotations the chart has defined.

With the current notifications, is hard to be aware of what exactly was
deployed, as just the Helm chart revision is included in the payload.
If I wanted to know what specific change (or changeset) has been rolled
out, it wouldn't be possible with the current setup. A workaround could
be to abuse the chart `version` semver, but of course with several
drawbacks, like needing to keep a 1-1 relationship between the char and
app versions, having to come up with some specific encoding, having it
to decode on the other end if a generic webhook receiver has been
configured, and just probably being a bad practice.

It's probably reasonable to be able to plug some arbitrary data into
the event delivered by Flux, specially considering that the Helm charts
already provide annotations for this.

By including the chart annotations as part of the metadata, users can
enrich their notifications as they wish by including the data they
consider necessary for their own use cases.

Doing it with the chart annotations, the user experience doesn't
change, as the chart needs to be updated for making a release
anyways, and the data can be set at that point; or just left it
empty otherwise if it's not needed.

The annotations must be in string:string format according to the Helm
specification itself, so no complex nested structures are allowed.
Prior to these changes, if nested annotations are specified in the
chart, the Helm upgrade already fails with no registered event, so
there's no check done regarding this matter.

Signed-off-by: Julen Pardo <julen.pardo@datarobot.com>
@eljulians eljulians force-pushed the julenpardo/chart-annotations-notification-metadata branch from d82ad5e to aec55b2 Compare August 10, 2022 12:26
@stefanprodan stefanprodan requested a review from hiddeco August 10, 2022 12:56
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @julenpardo for this contribution.

I think we need to prefix all keys with helm to avoid collisions in notification-controller. This PR will have to wait till end of August as Hidde is in vacation and he will probably want this to be merged into the dev branch.

@eljulians
Copy link
Author

@stefanprodan thanks a lot for such a quick response!

Should we then wait for Hidde to discuss how to prefix the keys?

@eljulians eljulians marked this pull request as ready for review August 22, 2022 11:27
@hiddeco
Copy link
Member

hiddeco commented Aug 26, 2022

Thank you for your contribution @julenpardo.

I am interested in adding this to the current refactoring work which lives in dev, but not all events have materialized there yet (which is something I am working on in addition to extensive test coverage now that I have returned). Once my PR is up which adds the majority of the events, I will circle back to this and see how we can properly pass on and/or prefix the data.

@eljulians
Copy link
Author

Thanks a lot @hiddeco for checking it and the heads up!

@Delorien84
Copy link

Hello, this MR has been sitting here for a long time. This feature would be great, as we have same use case as @julenpardo.

@matheuscscp
Copy link
Member

Hi @julenpardo thanks for putting this together, it's gonna be very useful!

A question: Where exactly do these chart annotations come from? Do they come from the HelmRelease .spec, .metadata, or .spec.chart.spec, or somewhere else?

I would really appreciate a way in which I could specify metadata in my HelmRelease manifest, like this: #682

Is your intention to also cover something like that or very similar? Could you please provide an example on where would I put my annotations/metadata exactly?

Thanks one more time!!

@moritzschmitz-oviva
Copy link

moritzschmitz-oviva commented Apr 26, 2024

A question: Where exactly do these chart annotations come from? Do they come from the HelmRelease .spec, .metadata, or .spec.chart.spec, or somewhere else?

Probably this PR revolves around the chart annotations in the Chart.yaml, like here.

I also made another comment in #682.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants